Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Font Library: unregister font collection #54701

Merged
merged 28 commits into from
Jan 8, 2024

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented Sep 21, 2023

Closes #54697

What?

Adds a function that enable extenders to unregister a font collection from the library.

Why?

There could be cases where extenders need to make a font collection unavailable.

How?

By implementing the function wp_unregister_font_collection( $id ), extenders can make a particular font collection not loaded.

Testing instructions

Try disabling the default font collection by adding this in a plugin code or in your block theme functions.php:

wp_unregister_font_collection ( 'default-font-collection' );

Expected result:

The default font collection (Google Fonts) should not load in the frontend:

Before After
image image

@github-actions
Copy link

github-actions bot commented Sep 21, 2023

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ phpunit/tests/fonts/font-library/wpFontLibrary/base.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/unregisterFontCollection.php
❔ lib/experimental/fonts/font-library/class-wp-font-library.php
❔ lib/experimental/fonts/font-library/font-library.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontCollections.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getFontsDir.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/getMimeTypes.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/registerFontCollection.php
❔ phpunit/tests/fonts/font-library/wpFontLibrary/setUploadDir.php

@matiasbenedetto matiasbenedetto marked this pull request as ready for review September 21, 2023 19:25
@pbking
Copy link
Contributor

pbking commented Sep 21, 2023

This didn't seem to work as I expected and I'm not sure why.

The UI changed as desired. I no longer saw the 'Install Fonts' tab as expected. But only if I ran the code early enough (I tried putting it in an 'init' function and it didn't change, but putting it outside of any functions made it work as expected).

However, if I called WP_Font_Library::get_font_collections() immediately after I still got that collection from the list.

@mikachan mikachan added [Feature] Typography Font and typography-related issues and PRs Needs PHP backport Needs PHP backport to Core [Type] Enhancement A suggestion for improvement. labels Sep 22, 2023
*/
function wp_unregister_font_collection( $collection_id ) {
add_action(
'init',
Copy link
Contributor

@costdev costdev Oct 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious about why this needs to be done in an init action within the function. Is this to ensure that WP_Font_Family is initialized beforehand? If so, it might be better that the docs just let extenders know this must occur in the init action or later, or even a _doing_it_wrong() if ! did_action( 'init' ) && ! doing_action( 'init' ).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because we would need to be sure that all the font collections registered to call wp_register_font_collection() were already registered so we can unregister them using wp_unregister_font_collection(). Is there a better way to be sure that all the font collections are already registered?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the latest commit, I removed the reliance on the hooks by keeping track of the unregistered font collection using an array of collection IDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the @spacedmonkey comments, I changed the approach following this example.

@matiasbenedetto
Copy link
Contributor Author

matiasbenedetto commented Oct 11, 2023

This didn't seem to work as I expected and I'm not sure why.
The UI changed as desired. I no longer saw the 'Install Fonts' tab as expected. But only if I ran the code early enough (I tried putting it in an 'init' function and it didn't change, but putting it outside of any functions made it work as expected).
However, if I called WP_Font_Library::get_font_collections() immediately after I still got that collection from the list.

Thanks for the review!

The latest changes should fix this. I removed the reliance on the init hook by keeping track of the unregistered font collection using an array of collection IDs. b7c9113

@spacedmonkey
Copy link
Member

What happens if there are no collections registered? How is this handled? Can we have unit tests for that?

Comment on lines 92 to 94
public static function unregister_font_collection( $collection_id ) {
self::$unregistered_collection_ids[] = $collection_id;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a weird pattern. Could we not just unset the value in self::$collections?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the example. Very useful. I implemented that in the latest commits.

@matiasbenedetto
Copy link
Contributor Author

What happens if there are no collections registered? How is this handled? Can we have unit tests for that?

When no collections are registered, get_font_collections() returns an empty array. This is the unit test:

public function test_should_get_an_empty_list() {
$font_collections = WP_Font_Library::get_font_collections();
$this->assertEmpty( $font_collections, 'Should return an empty array.' );
}

Co-authored-by: Vicente Canales <1157901+vcanales@users.noreply.github.com>
Co-authored-by: Grant Kinney <creativecoder@users.noreply.github.com>
Copy link
Contributor

@creativecoder creativecoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to be working well for me

  • ✅ I can unregister the default collection with wp_unregister_font_collection ( 'default-font-collection' );
    • The "Install Fonts" tab no longer displays in the Fonts modal
    • WP_Font_Library::get_font_collections() returns an empty array
  • ✅ If I try to unregister a non-existent collection, I see a notice in the PHP error log
  • ✅ If I try to register a collection with an existing id, I see a notice in the PHP error log

I left some suggestions for the tests, otherwise I think this is good to go.

@matiasbenedetto
Copy link
Contributor Author

The PR seems ready to go, please could you give it another review?

Copy link
Contributor

@jffng jffng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified:

  • Unregister an existing collection
  • Warnings are thrown when attempting to register an existing collection or unregister a non-existing collection
  • Unit tests incorporate the feedback

@matiasbenedetto matiasbenedetto merged commit 302c148 into trunk Jan 8, 2024
55 checks passed
@matiasbenedetto matiasbenedetto deleted the try/unregister-font-collection branch January 8, 2024 13:31
@github-actions github-actions bot added this to the Gutenberg 17.5 milestone Jan 8, 2024
@bph bph added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jan 16, 2024
@youknowriad youknowriad added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Typography Font and typography-related issues and PRs Needs Dev Note Requires a developer note for a major WordPress release cycle [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Font Library: unregister font collection
10 participants